Skip to content

Fix import errors introduced by sklearn 1.5 #229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
May 26, 2024
Merged

Fix import errors introduced by sklearn 1.5 #229

merged 3 commits into from
May 26, 2024

Conversation

agoscinski
Copy link
Collaborator

@agoscinski agoscinski commented May 22, 2024 •
edited
Loading

With sklearn version 1.5 the check_pandas_support function has been moved to util._optional_dependencies. This has been adapted in this PR. I would still try to be backwards compatible to 1.* so I put an if then else in the import

Contributor (creator of PR) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

For Reviewer

  • CHANGELOG updated if important change?

📚 Documentation preview 📚: https://scikit-matter--229.org.readthedocs.build/en/229/

@agoscinski agoscinski changed the title Fix import errors introduces by sklearn 0.1.5 Fix import errors introduced by sklearn 0.1.5 May 22, 2024
@agoscinski agoscinski force-pushed the fix-sklearn-1.5-error branch 3 times, most recently from df70dad to a6705e8 Compare May 22, 2024 20:41
@agoscinski agoscinski requested a review from PicoCentauri May 22, 2024 20:41
@agoscinski agoscinski force-pushed the fix-sklearn-1.5-error branch from a6705e8 to 7dd4892 Compare May 22, 2024 20:42
@PicoCentauri
Copy link
Collaborator

Some issues seems to be unrelated. Do you have an idea @agoscinski ?

@agoscinski agoscinski force-pushed the fix-sklearn-1.5-error branch from 7dd4892 to 08393a0 Compare May 22, 2024 21:44
@agoscinski
Copy link
Collaborator Author

There is a test error in the DCH test. I assume we test something that is not well defined like an orthogonal vector that can have some sign flips, because the difference in the regression test is a sign flip. Then with a change in numpy/scipy version the sign can easily change. A projection of the function that needs to be interpolated it looks already not so numerically stable
debug_dch

But honestly I don't know what exactly is happening. We could replace these random points with a smooth high dimensional function, and make this regression test a bit more extensive (testing all points not only one), so we can be sure that we did not just fix this by picking a point that works.

If we need a fast fix (to not block other PRs), I would put a warning now into the code, because if something was broken, then it was already broken before the sklearn 1.5.0 release

@agoscinski agoscinski changed the title Fix import errors introduced by sklearn 0.1.5 Fix import errors introduced by sklearn 1.5 May 22, 2024
@ceriottm
Copy link
Collaborator

There is a test error in the DCH test. I assume we test something that is not well defined like an orthogonal vector that can have some sign flips, because the difference in the regression test is a sign flip. Then with a change in numpy/scipy version the sign can easily change. A projection of the function that needs to be interpolated it looks already not so numerically stable debug_dch

But honestly I don't know what exactly is happening. We could replace these random points with a smooth high dimensional function, and make this regression test a bit more extensive (testing all points not only one), so we can be sure that we did not just fix this by picking a point that works.

If we need a fast fix (to not block other PRs), I would put a warning now into the code, because if something was broken, then it was already broken before the sklearn 1.5.0 release

Let me test this to see if I can understand what's the problem. I don't think we should merge when tests are broken - if there's an issue with the test logic we should change the logic.

@agoscinski
Copy link
Collaborator Author

agoscinski commented May 23, 2024 •
edited
Loading

Let me test this to see if I can understand what's the problem. I don't think we should merge when tests are broken - if there's an issue with the test logic we should change the logic.

My point is that, this PR has nothing to do with this issue. It just made it visible. But if you can fix it fast, sure go ahead. I would just not wait another week, since other PRs are in the pipeline.

@ceriottm
Copy link
Collaborator

Fixed the test.

@agoscinski
Copy link
Collaborator Author

I am missing something. I can understand that PCA has a sign flip because of orthogonalization but how can the residual be negative if it was fitted on the same data (self.T) even if PCA has a sign flip.

@agoscinski
Copy link
Collaborator Author

agoscinski commented May 24, 2024 •
edited
Loading

I think my misunderstanding came from ignoring the fact that the convex hull is created on self.T and self.y thereby the residuals in a subspace (of self.T) can be nonzero . And then a flip in the sign of the principal components can actually effect on the residuals

@agoscinski agoscinski force-pushed the fix-sklearn-1.5-error branch from c7c1973 to f7ded60 Compare May 24, 2024 05:34
@agoscinski
Copy link
Collaborator Author

agoscinski commented May 24, 2024 •
edited
Loading

Thanks @ceriottm for fixing it! I just rephrased the commit message a bit. Will merge as soon as CI passes

@agoscinski agoscinski force-pushed the fix-sklearn-1.5-error branch 5 times, most recently from cb4f40a to 445ee87 Compare May 26, 2024 18:10
agoscinski and others added 3 commits May 26, 2024 20:13
@agoscinski
Fix check_pandas_support import error introduced by sklearn 1.5.0
18bca5f 
With sklearn version 1.5.0 the check_pandas_support function has been
moved to util._optional_dependencies. This has been adapted in our code.
@ceriottm @agoscinski
Fix in test_dch.py failing test_selected_idx_and_scores due to sign f…
2447d0f 
…lip(s) in PCA

With version changes of the of sklearn the PCA output can have sign
flips of the principal components. Now we only compare the absolute
value to make the test invariant to such sign changes.
@agoscinski
Update the changelog for sklearn 1.5.0 update
4c50009
@agoscinski agoscinski force-pushed the fix-sklearn-1.5-error branch from 445ee87 to 4c50009 Compare May 26, 2024 18:13
@agoscinski agoscinski merged commit a5e815e into main May 26, 2024
@agoscinski agoscinski deleted the fix-sklearn-1.5-error branch May 26, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PicoCentauri PicoCentauri PicoCentauri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@agoscinski @PicoCentauri @ceriottm